Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

@claude check this PR

Code review

Found 2 issues:

1. Hook returning None (no-opinion) is now treated as a hard deny — breaking existing hook contracts

The original loop used if controller_permission is not None: return bool(controller_permission), which skips hooks that return None and only denies on an explicit falsy non-None value. The new loop uses if not controller_permission: return bool(controller_permission), so any hook that returns None (the conventional "I have no opinion, let the next hook decide") will now immediately return False and deny access. Every existing has_permission hook that returns None or omits a return statement will silently break.

2. Denial by a later hook can be undone by an earlier hook in the same loop — anti-escalation snapshot is not cumulative

org_permissions is captured once before the loop. The guard if org_permissions.get(key) == 0 and value == 1 only blocks re-enabling a bit that was originally 0 before any hook ran. If hook B (executed first, because reversed) sets permissions["delete"] = 0, hook A (executed second) can freely set it back to 1 — because org_permissions.get("delete") is 1, not 0, so the guard does not fire. The fix is to maintain a cumulative denied_keys set that grows as hooks deny bits, and enforce it after every hook call.

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

Code review

Found 1 issue:

1. Anti-escalation guard fails to revert permission keys newly added by hooks — a hook can set a permission key that was absent from the original role-based permissions (e.g. permissions["delete"] = 1 when delete was not granted), and the guard will not revert it because org_permissions.get(key) returns None, not 0, so None == 0 is False. This directly violates the stated invariant in the docstring: "Controllers can only deny permission, they can not explicitly grant any permission that wasn't already present." The guard should check org_permissions.get(key, 0) == 0 (or equivalently, check key not in org_permissions or org_permissions[key] == 0) to cover keys absent from the original dict.

🤖 Generated with Claude Code

_{If this code review was useful, please react with 👍. Otherwise, react with 👎.}

@claude can fix the issue